[16.0][REF] hr_timesheet_begin_end: More extensible, use compute instead of onchange #692
Conversation
carmenbianca
force-pushed
the
16.0-refactor-hr_timesheet_begin_end
branch
2 times, most recently
from
299a3b8 to
ae7ea83
Compare
carmenbianca
force-pushed
the
16.0-refactor-hr_timesheet_begin_end
branch
from
ae7ea83 to
0423bc6
Compare
carmenbianca
force-pushed
the
16.0-refactor-hr_timesheet_begin_end
branch
from
0423bc6 to
1212f36
Compare
| self._validate_start_before_stop() | ||
| self._validate_unit_amount_equal_to_time_diff() | ||
| self._validate_no_overlap() |
There was a problem hiding this comment.
this should be guarded with:
if not self.project_id:
returnThere was a problem hiding this comment.
added some such guard using a filter.
should this add project_id to the constraint? if yes, should all the fields from _overlap_domain also be added to the constraint?
return [
("id", "!=", self.id),
("employee_id", "=", self.employee_id.id),
("date", "=", self.date),
("time_start", "<", self.time_stop),
("time_stop", ">", self.time_start),
]
i think technically yes, but i am rather wary about constraints with a lot of fields.
| self._validate_no_overlap() | ||
|
|
||
| @api.model | ||
| def _hours_from_start_stop(self, time_start, time_stop): |
There was a problem hiding this comment.
what if one of time_start or time_stop is False?
There was a problem hiding this comment.
I'm not sure this should happen because Floats default to 0 in Odoo.
| unit_amount = fields.Float( | ||
| compute="_compute_unit_amount", | ||
| store=True, | ||
| readonly=False, |
There was a problem hiding this comment.
shouldn’t this have an inverse function instead of readonly=False? the documentation doesn’t mention readonly=False (although the odoo code contains some of those) but states that to allow writing to a computed field, the inverse parameter must be used. this would also allow to correctly compute time_stop from time_start and unit_amount (but i don’t know whether this is actually useful).
There was a problem hiding this comment.
not needed, i think. compute="_method", store=True, readonly=False is a pattern in odoo to compute whenever the requirements change, but to allow the user to override subsequently.
| def create(self, vals_list): | ||
| # This is a workaround for a bizarre situation: if a line is created | ||
| # with a time range but WITHOUT defining unit_amount, then you would | ||
| # expect unit_amount to be computed from the range. But for some reason, | ||
| # this never happens, and it is instead set to 0. Subsequently the | ||
| # constraint _validate_unit_amount_equal_to_time_diff kicks in and | ||
| # raises an exception. | ||
| # | ||
| # As a workaround, we add unit_amount with the correct value if it isn't | ||
| # in the vals_list. A better workaround might be to magically detect | ||
| # that unit_amount is not yet correctly computed in the constraint, and | ||
| # to skip the constraint if so. | ||
| self._add_unit_amount_to_vals_list(vals_list) | ||
| return super().create(vals_list) |
There was a problem hiding this comment.
i dug deeper into the problem and it comes from the fact that the analytic module defines unit_amount with default=0.0. when there is no default, stored computed fields are correctly computed at creation time, before constrains.
the only way to restore this behavior is to set default=None on the field. the compute function should then correctly set the field to its default value of 0.0 when needed.
i tried using a callable as the default value, but it must return a value, and any falsy value seems to be converted to 0, thus not resolving the problem.
There was a problem hiding this comment.
i think it's fine to set default=None then.
Timesheet lines are characterised by having a project. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
The separation of checks into separate methods is needed because I want to disable one check in another module. This makes the module more extensible. The unit_amount_from_start_stop method also makes the module more extensible. I have also moved the onchange to the top of the file, according to the OCA contribution guidelines. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
carmenbianca
force-pushed
the
16.0-refactor-hr_timesheet_begin_end
branch
from
1212f36 to
23acc70
Compare
Instead of using onchange, which is less convenient in Odoo 16. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
carmenbianca
force-pushed
the
16.0-refactor-hr_timesheet_begin_end
branch
from
23acc70 to
55c67c2
Compare
This is a tricky one. _Technically_ there is nothing that makes hr_timesheet_begin_end incompatible with the other modules here. However, when the project_id on a timesheet line is changed, unit_amount is rerecomputed. This has some consequences. In the sale_timesheet tests, the project_id of a timesheet line is (sometimes?) recomputed. Subsequently, unit_amount is recomputed and set to 0 when it _shouldn't_ be. Under a normal flow this wouldn't be a problem; time_start and time_stop would be populated to correctly recompute unit_amount. But in the tests, they are not. One solution is to, in addition to not recomputing the unit_amount of non-timesheet lines, not recompute the unit_amount of timesheet lines whose time_start and time_stop are both set to 00:00. However, this means that resetting these values to 00:00 does not correctly set unit_amount back to 0, which seems erroneous to me. Marking this module as a rebel module seems like the best course of action to me, even though it would be preferable to test this in conjunction with the other modules. Signed-off-by: Carmen Bianca BAKKER <[email protected]>
|
I added |
There was a problem hiding this comment.
just a comment about a (too long) comment. otherwise lgtm! thanks @carmenbianca!
| # This default is a workaround for a bizarre situation: if a line is | ||
| # created with a time range but WITHOUT defining unit_amount, then you | ||
| # would expect unit_amount to be computed from the range. But this never | ||
| # happens, and it is instead set to default value 0. Subsequently the | ||
| # constraint _validate_unit_amount_equal_to_time_diff kicks in and | ||
| # raises an exception. | ||
| # | ||
| # By setting the default to None, the computation is correctly | ||
| # triggered. If nothing is computed, None falls back to 0. |
There was a problem hiding this comment.
i think that the comment can be simpler. something like:
| # This default is a workaround for a bizarre situation: if a line is | |
| # created with a time range but WITHOUT defining unit_amount, then you | |
| # would expect unit_amount to be computed from the range. But this never | |
| # happens, and it is instead set to default value 0. Subsequently the | |
| # constraint _validate_unit_amount_equal_to_time_diff kicks in and | |
| # raises an exception. | |
| # | |
| # By setting the default to None, the computation is correctly | |
| # triggered. If nothing is computed, None falls back to 0. | |
| # removing the default of 0.0 to ensure it is computed when not | |
| # provided. if not computed (because project_id is False), None is | |
| # automatically converted to 0. |
Needed for #691
Internal task: T12630